Codecov Report

Merging #56 (eabaab6) into master (4ff6e45) will decrease coverage by 3.07%.
The diff coverage is 40.74%.

@@            Coverage Diff             @@
##           master      #56      +/-   ##
==========================================
- Coverage   85.44%   82.36%   -3.08%     
==========================================
  Files          18       18              
  Lines         371      397      +26     
==========================================
+ Hits          317      327      +10     
- Misses         28       40      +12     
- Partials       26       30       +4     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ff6e45...eabaab6. Read the comment docs.

Hi @arthurclement-lab49
Thanks for your PR but the optimistic lock is a specific need and each user may want to implement it differently, it's not a generic feature for everyone, so maybe it's good to keep everything simple and let users implement it by themselves needs, for example, such a DefaultModel(that include the version field) is good for you, so maybe its good write it as a Custom DefaultModel in your project and use it as your DefaultModel in your models.

PRs that have more than 10line changes, it would be good to raise a feature request issue for them to talk about the feature before creating a PR, this will save your time.

I'll close this PR, feel free to reopen it if needed.

Hi @mehran-prs

Thanks for looking into this. Can I just ask you look into my observations below before you definitely close this PR.

Optimistic locking is an essential feature for whatever project I have to implement, and I'm sure lots of other users, and although mongodb now has transactions, you dont always want to use them, especially when updating a single document.
A project like https://mongoosejs.com/docs/guide.html#versionKey which is popular in the js world has it supported out of the box given it is such a common feature.

I appreciate that having it in baked in the defaultModel is perhaps too much as it will indeed add a version field that you might not want for your documents (although there is always the option of redefining your own DefaultModel as you mention).
A suitable alternative that you might consider then is to leave the VersionField struct in the code but not included in the default model. With this way, one would simply need to add the version field explicitely to his model to get optimistic locking in a simple manner :

type User struct {
	mgm.DefaultModel `bson:",inline"`
	VersionField     `bson:",inline"`
}

The PR is actually flexible when it comes to this version field though as you just need to implement the Versionable interface.

But you also need to consider the most important part of the PR which is the update function of the operation.go file and will actually check whether the document you are saving has the right matching version.
Please consider at least this part of the PR as we can indeed work around the rest in the user land. Without this, users cannot just implement optimistic locking themselves unfortunately as the check needs to happen at the time of the update itself which is inside your library. If you dont want to merge it in, there will be no other choice than fork this library.
Thanks

Hi @arthurclement-lab49
You are right. It's good to implement this feature in our library as it needs some changes in the library itself.
I have reviewed your PR and left some comments for you.
Thanks for your PR again.

Hi mehran-prs

I've implemented your feedback. Its probably lacking a proper test at this stage to make sure everything works as expected. Will try to get to it later.

Let's say a user implements Versionable using the Version field of type time.Time.
When they create a document by calling it Create(), the version field value will be 0001-01-01 00:00:00 which is not what we wanted. I think it's good to set the document's first version at creation time by calling doc.IncrementVersion in the Create method. it makes sense for the version of type int too, as we know the first version of a document should be 1 instead of zero.

After this change, we don't need to check if the version is zero in the Update method, but it's a good idea to keep that check to cover documents that already have been created without the optimistic lock feature.

Let's say a user implements Versionable using the Version field of type time.Time. When they create a document by calling it Create(), the version field value will be 0001-01-01 00:00:00 which is not what we wanted. I think it's good to set the document's first version at creation time by calling doc.IncrementVersion in the Create method. it makes sense for the version of type int too, as we know the first version of a document should be 1 instead of zero.

After this change, we don't need to check if the version is zero in the Update method, but it's a good idea to keep that check to cover documents that already have been created without the optimistic lock feature.

Yes ok, version will therefore start at 1 for the int strategy with this change which is ok, just need to be aware of it.

I've also done a couple of other changes as described in the last commit message :

Name version field name Version_ to avoid collisions

As discussed

In operation.update(), capture Version before hooks in case updatedAt is used as versionable field

Necessary if you want to use udpatedAt as the version field. I have thought more about this case and think that it would require even more changes if we wanted to support this case out of the box, eg have a struct DateFieldsVersionable that would implement the Versionable interface. Perhaps we can envisage this later but its worth keeping in mind.

In operation.create(), if model is versionable and zero, increment version so it gets initialized in DB

As discussed

Rename GetVersionFieldName to GetVersionBsonFieldName to make it clear it is the bson field name.

I think it is clearer

Rename GetVersion() to Version() as it is more idiomatic

go uses the simpler form for getters so I aligned with that.